Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

modify roll test=develop #25321

Merged
merged 4 commits into from
Jul 13, 2020
Merged

Conversation

yaoxuefeng6
Copy link
Contributor

@yaoxuefeng6 yaoxuefeng6 commented Jul 1, 2020

PR types

Others

PR changes

APIs

Describe

According to paddle 2.0 standard
1, change attr name 'dim' to 'axis'
2, change api input name 'input' to 'x'
3, add check axis value in python code and related ut.
4,change example code to imperative mode

@paddle-bot-old
Copy link

paddle-bot-old bot commented Jul 1, 2020

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@@ -104,7 +104,7 @@ def flip(input, dims, name=None):
return out


def roll(input, shifts, dims=None):
def roll(input, shifts, axis=None, name=None):
"""
Copy link
Contributor

@jzhang533 jzhang533 Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be : paddle.tensor.roll(x, shifts, axis=None, name=None)
according to the latest argument convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -117,7 +117,7 @@ def roll(input, shifts, dims=None):
input (Variable): The input tensor variable.
shifts (int|list|tuple): The number of places by which the elements
of the `input` tensor are shifted.
dims (int|list|tuple|None): Dimentions along which to roll.
axis (int|list|tuple|None): Dimentions along which to roll.
Copy link
Contributor

@jzhang533 jzhang533 Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dimensions -> axis
上面的简介部分:``Roll the input tensor along ...'' 需要换一种说法, character level copy是不可以的。
示例代码中用paddle.enable_imperative()开启动态图模式,方便未来默认动态图的时候统一调整示例代码。

"Attr(dims[%d]) is out of range, It's expected "
"to be in range of [-%d, %d]. But received Attr(dims[%d]) = %d.",
"Attr(axis[%d]) is out of range, It's expected "
"to be in range of [-%d, %d]. But received Attr(axis[%d]) = %d.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python端也增加对axis范围的检查,防止打印出来call stack。

123malin
123malin previously approved these changes Jul 8, 2020
Copy link
Contributor

@123malin 123malin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@jzhang533 jzhang533 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@liym27 liym27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for the changes of Attrs of roll op, but please node that it will cause that Paddle inference library of 2.0 version to load model trained by the old version failed.

@yaoxuefeng6 yaoxuefeng6 merged commit c42d662 into PaddlePaddle:develop Jul 13, 2020
@yaoxuefeng6 yaoxuefeng6 deleted the new_roll branch July 13, 2020 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants